Skip to content

Silo admin endpoints for user logout + listing tokens and sessions #8479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Jun 30, 2025

The idea here is that to disable a user's access to the system, admins first disable that user's ability to log in on the IdP side and then hit this endpoint to remove all of their existing credentials on our end. The centerpiece is the logout endpoint, but I added the endpoints for listing sessions and tokens because someone pointed out you really want to see those come back empty after logout. They're also kind of useful anyway. Then I added user_view just because it wouldn't make sense to have token and session list endpoints hanging off /v1/users/{user_id} without having that defined.

  • Add /v1/users/{user_id}/logout that deletes all of the user's tokens and sessions
  • Add new authz resource SiloUserAuthnList letting us authorize that action for silo admins specifically (can't use silo modify because fleet collaborator and admin get that on all silos)
  • Update IAM policy test
  • Test that logout deletes tokens and the right perms are enforced
  • Test that logout deletes sessions and the right perms are enforced
  • Add user_view and user_token_list and user_session_list endpoints for symmetry and to give the admin a warm fuzzy feeling when they see that the tokens and sessions are in fact gone (also makes testing a little cleaner)
  • Fix session list including expired sessions (and test it)
  • Think about whether we need to do something about dueling admins issues, i.e., what if the person you're trying to disable are themselves a silo admin and they log everyone else out of the silo. The only solution I can think of off the top of my head is an operator-level version of this endpoint that can be used by a user outside of the silo in question.

@david-crespo david-crespo force-pushed the silo-user-logout branch 2 times, most recently from 109676e to 276db6b Compare July 1, 2025 22:30
@david-crespo david-crespo changed the title Silo admin endpoint for user logout Silo admin endpoint for user logout + listing tokens and sessions Jul 2, 2025
@david-crespo david-crespo changed the title Silo admin endpoint for user logout + listing tokens and sessions Silo admin endpoints for user logout + listing tokens and sessions Jul 2, 2025
"modify" if "admin" on "parent_silo";

# A silo admin can list a user's tokens and sessions.
"list_children" if "admin" on "parent_silo";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little funny because I'm using list_children to determine whether you can list sessions and tokens (only self and silo admin can) but then I'm using modify on the list itself to determine whether you can do the logout delete-all operation. It works fine, but it goes slightly against the grain of how I know it's supposed to work. We just don't have many delete all type things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that seems resonable to me, though I'm far from the most knowledgeable about how we tend to model stuff in Polar... @davepacheco might have an opinion?

@david-crespo david-crespo marked this pull request as ready for review July 2, 2025 23:11
@ahl ahl requested a review from hawkw July 15, 2025 17:02
Comment on lines +178 to +184
// TODO: unlike with tokens, we do not have expiration time here,
// so we can't filter out expired sessions by comparing to now. In
// the authn code, this works by dynamically comparing the created
// and last used times against now + idle/absolute TTL. We may
// have to do that here but it's kind of sad. It might be nicer to
// make sessions work more like tokens and put idle and absolute
// expiration time right there in the table at session create time.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the major outstanding issue. I will probably make the fix a separate PR on top of this one because it's an actual change, not just an addition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, I like the idea of the proposed refactor --- generally it seems nicer to represent things as deadlines, I think. I agree that it's a big enough change that it could be done subsequently. In the meantime, might we want to cruft together a thing that filters such sessions out of the returned Vec in Rust code?

Do you think it's worth opening an issue to track that, in addition to/instead of the TODO comment?

Comment on lines +671 to +672
// TODO: does it make sense to use a single resource to represent both user
// sessions and tokens? it seems silly to have two identical ones
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how would that work? Is that a question you want to answer before merging this branch?

Copy link
Contributor Author

@david-crespo david-crespo Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status quo is the single resource version (I meant resource from the POV of authz). I think it would be simple to have two instead, I would just make a second one and name one tokens and one sessions, and use each as appropriate. Since that can be done any time we need to distinguish the two (we may never have to) I am inclined to solve this by deleting the comment.

"modify" if "admin" on "parent_silo";

# A silo admin can list a user's tokens and sessions.
"list_children" if "admin" on "parent_silo";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that seems resonable to me, though I'm far from the most knowledgeable about how we tend to model stuff in Polar... @davepacheco might have an opinion?

Comment on lines +178 to +184
// TODO: unlike with tokens, we do not have expiration time here,
// so we can't filter out expired sessions by comparing to now. In
// the authn code, this works by dynamically comparing the created
// and last used times against now + idle/absolute TTL. We may
// have to do that here but it's kind of sad. It might be nicer to
// make sessions work more like tokens and put idle and absolute
// expiration time right there in the table at session create time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, I like the idea of the proposed refactor --- generally it seems nicer to represent things as deadlines, I think. I agree that it's a big enough change that it could be done subsequently. In the meantime, might we want to cruft together a thing that filters such sessions out of the returned Vec in Rust code?

Do you think it's worth opening an issue to track that, in addition to/instead of the TODO comment?

opctx.authorize(authz::Action::Modify, authn_list).await?;

use nexus_db_schema::schema::device_access_token;
diesel::delete(device_access_token::table)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Occurs to me we could do a soft-delete-like thing by mutating the expiration time to now, though that feels kind of sketchy to me as the soft-delete would disturb the record of what that token was originally supposed to be... just hard deleting them is probably less goofy!

Comment on lines +3114 to +3116
/// Delete all of user's tokens and sessions
#[endpoint {
method = POST,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be goofy of me: it seems a bit strange that this is a POST but the description in the API docs begins with the verb "delete". On the other hand, since there are separate resources for sessions and tokens which are both deleted in one operation by this route, I'm not sure if there's a natural way to make this be a DELETE...obviously it shouldn't be DELETE /v1/users/{id}, but it's equally awkward to make it DELETE /v1/users/{id}/sessions since it also deletes tokens. Gah. Okay.

Maybe it should remain a POST but the slug could be changed to just say "logs out the user", with additional doc text following that that states what logging out means is "sessions and tokens get deleted"? I dunno. On the other hand, that just obscures what it actually does in the interest of my feeling Vaguely Weird about the use of the word "delete" in a route that isn't an HTTP DELETE...

🤷‍♀️

Comment on lines +330 to +336
self.datastore()
.silo_user_tokens_delete(opctx, &authz_authn_list)
.await?;

self.datastore()
.silo_user_sessions_delete(opctx, &authz_authn_list)
.await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so, it's worth noting that this is not atomic: if the silo_user_tokens_delete query succeeds, but the silo_user_sessions_delete query fails, we could return a 500 error and leave the user in a half-logged-in state where they have no auth tokens but any existing console sessions still exist. I don't actually know whether that's huge a problem or not: is anything else liable to get confused if it encounters a user in such a state, and how bad are the consequences of that? AFAICT we won't get permanently stuck in such a state, since this is idempotent: we can try to log the user out again and it won't matter that we've already deleted their tokens, right?

If it is an issue we could make this a transaction, so the whole logout operation atomically succeeds or fails. I don't think this is necessary but wanted to make sure we'd thought through it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about this a bit. Basically the half-deleted state is fine from an everything working POV, and it's at least in the direction of what the user wanted to happen, and they know from the error response that it didn't fully work (though it's likely they expect it not to have worked at all, unless the error indicates otherwise). I lean against a transaction but I will at least add a comment showing this is deliberate.

) {
let testctx = &cptestctx.external_client;

// create a user have a user ID on hand to use in the authn_as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sentence seems to be missing one or more words...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants